Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: Wrap the new FormDesignerNavigation with AppContext #14884

Merged
merged 10 commits into from
Mar 12, 2025
Merged

refactor: Wrap the new FormDesignerNavigation with AppContext #14884

merged 10 commits into from
Mar 12, 2025
+98 −51

Conversation

mlqn
Copy link
Contributor

@mlqn mlqn commented Mar 5, 2025 •
edited by coderabbitai bot
Loading

Description

  • Wrapped the new FormDesignerNavigation with AppContext to allow it to access the state of the selected layout set

Related Issue(s)

  • PR itself

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)

Summary by CodeRabbit

  • New Features

    • Enhanced the form editor experience with context-aware navigation that guides users when no layout is selected.
    • Introduced flexible layout management, allowing users to clear their current layout selection.

  • Refactor

    • Streamlined the logic for layout selection and navigation, resulting in a more intuitive and simplified user interface.

@coderabbitai coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 5, 2025 •
edited
Loading

📝 Walkthrough

Walkthrough

The update refactors both tests and component logic by removing legacy feature flag checks and the FormDesignerNavigation component. In the router files, mocks and conditional renderings based on the task navigation feature flag have been eliminated. The shared hook now returns an additional removeSelectedFormLayoutSetName method with corresponding test suite modifications. Updates in the ux-editor package add this new method to the app context and its mocks, and the SubApp component now conditionally renders based on feature flag status using a newly defined subcomponent.

Changes

File(s) Change Summary
frontend/app-development/router/routes.tsx
frontend/app-development/router/routes.test.tsx		 Removed mocks for FormDesignerNavigation and shouldDisplayFeature; eliminated imports and conditional rendering logic related to task navigation feature flags.
frontend/packages/shared/src/hooks/useSelectedFormLayoutSetName.ts
.../useSelectedFormLayoutSetName.test.tsx		 Updated hook to include a new removeSelectedFormLayoutSetName method and adjusted the return structure; enhanced tests with cleanup for feature flags and added a test for non-existent layouts when the task navigation flag is enabled.
frontend/packages/ux-editor/src/AppContext.tsx
frontend/packages/ux-editor/src/SubApp.tsx
frontend/packages/ux-editor/src/SubApp.test.tsx
frontend/packages/ux-editor/src/testing/appContextMock.ts		 Added removeSelectedFormLayoutSetName to the app context interface and its mock; refactored SubApp by introducing a new App component for conditional rendering based on the task navigation feature flag; introduced a helper function in tests to render with providers.

Possibly related PRs

Suggested labels

team/studio-domain1, skip-manual-testing

Suggested reviewers

  • Jondyr
📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1a553c4 and 72be432.

📒 Files selected for processing (8)
  • frontend/app-development/router/routes.test.tsx (0 hunks)
  • frontend/app-development/router/routes.tsx (1 hunks)
  • frontend/packages/shared/src/hooks/useSelectedFormLayoutSetName.test.tsx (3 hunks)
  • frontend/packages/shared/src/hooks/useSelectedFormLayoutSetName.ts (1 hunks)
  • frontend/packages/ux-editor/src/AppContext.tsx (4 hunks)
  • frontend/packages/ux-editor/src/SubApp.test.tsx (1 hunks)
  • frontend/packages/ux-editor/src/SubApp.tsx (1 hunks)
  • frontend/packages/ux-editor/src/testing/appContextMock.ts (1 hunks)
💤 Files with no reviewable changes (1)
  • frontend/app-development/router/routes.test.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
  • GitHub Check: Build environment and run e2e test
  • GitHub Check: Typechecking and linting
  • GitHub Check: CodeQL
  • GitHub Check: Testing
🔇 Additional comments (23)
frontend/packages/ux-editor/src/testing/appContextMock.ts (1)

14-14: LGTM - Mock function added for new context method.

The addition of the removeSelectedFormLayoutSetName mock function aligns with the extended AppContextProps interface. This ensures test compatibility with the updated context.

frontend/packages/ux-editor/src/AppContext.tsx (3)

20-20: LGTM - Interface correctly extended with new method.

The AppContextProps interface has been properly updated with the new method for removing the selected form layout set name.

52-56: LGTM - Properly destructured new method from hook.

The destructuring assignment has been updated to include the new removeSelectedFormLayoutSetName method from the hook.

109-109: LGTM - Context value and dependencies updated.

The new method has been correctly added to both the context value object and the dependency array of the useMemo hook, ensuring proper reactivity when the method changes.

Also applies to: 125-125

frontend/packages/shared/src/hooks/useSelectedFormLayoutSetName.test.tsx (3)

12-13: LGTM - Required imports added for feature flag testing.

Appropriate imports added for the typed local storage and feature flag utilities needed for the new test case.

56-59: LGTM - Test cleanup properly implemented.

The afterEach hook now includes cleanup for feature flags in local storage, preventing test interference.

77-87: LGTM - Good test coverage for feature flag condition.

This test case properly verifies the behavior when the task navigation feature is enabled and no layout set is selected. It ensures the hook returns undefined instead of defaulting to the first layout set.

frontend/packages/ux-editor/src/SubApp.tsx (3)

4-7: LGTM - Imports updated for new conditional rendering.

The imports have been updated to support the new conditional rendering logic. Renaming App to FormDesigner improves code clarity given the new local App component.

15-24: LGTM - Clean implementation of conditional rendering.

The new App component implements the conditional rendering logic based on the feature flag status and context state. The approach is clean and follows React best practices.

This implementation successfully wraps the FormDesignerNavigation with the app context, allowing it to access the selected layout set state as described in the PR objectives.

28-30: LGTM - SubApp updated to use the new App component.

The SubApp component now correctly uses the new App component, maintaining the same interface while incorporating the enhanced rendering logic.

frontend/app-development/router/routes.tsx (1)

42-70: Clean and simplified UiEditor component implementation

The UiEditor component has been simplified by removing the conditional rendering logic for the FormDesignerNavigation component. This aligns with the PR objective of refactoring the FormDesignerNavigation to be wrapped with AppContext instead of being directly rendered here.

The handleLayoutSetNameChange function is properly maintained to keep the layout context updated, which ensures smooth communication between components.

frontend/packages/ux-editor/src/SubApp.test.tsx (7)

5-12: Good test setup with appropriate imports

The added imports provide the necessary tools for properly testing the new functionality around FormDesignerNavigation with AppContext.

16-16: Test ID for form navigation properly defined

Adding a dedicated test ID for the form navigation component makes it easier to verify its presence in the DOM.

22-31: Proper mocking of components and hooks

The mocks for useAppContext and FormDesignerNavigation are well implemented, providing the necessary test doubles to isolate the SubApp component behavior.

38-41: Feature toggle utils mocked correctly

Good approach to mock the feature toggle utils while preserving the actual implementation using requireActual.

45-45: Simplified test rendering with helper function

Using the new renderWithProviders helper function improves test readability.

51-65: Comprehensive test for FormDesignerNavigation rendering

This test effectively verifies that FormDesignerNavigation renders when the task navigation feature is enabled and no layout set is selected. It correctly mocks the feature flag and sets up the query client with appropriate app version data.

67-73: Well-implemented helper function for test rendering

The renderWithProviders function is a good abstraction that encapsulates the test setup complexity, making tests more readable and maintainable.

frontend/packages/shared/src/hooks/useSelectedFormLayoutSetName.ts (5)

4-4: Appropriate import for feature flag utils

Adding the import for FeatureFlag and shouldDisplayFeature enables the conditional behavior based on feature flags.

9-9: Enhanced hook API with removal capability

Adding the removeSelectedFormLayoutSetName method to the UseSelectedFormLayoutSetNameResult type enhances the hook's API, allowing components to clear the selected layout set when needed.

16-18: Feature flag check and conditional default logic

The implementation correctly checks for the TaskNavigation feature flag and sets the defaultLayoutSet to undefined when the feature is enabled. This ensures that users must explicitly select a layout set when using task navigation.

20-21: Updated local storage hook usage

The useLocalStorage hook usage is updated to destructure the third return value (removeSelectedFormLayoutSetName), which is essential for implementing the removal capability.

28-28: Properly exposing the remove method

The removeSelectedFormLayoutSetName method is correctly exposed in the returned object, making it available to consumers of this hook.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@github-actions github-actions bot added area/ui-editor Area: Related to the designer tool for assembling app UI in Altinn Studio. solution/studio/designer Issues related to the Altinn Studio Designer solution. frontend labels Mar 5, 2025
@codecov Codecov
Copy link

codecov bot commented Mar 6, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.85%. Comparing base (1a553c4) to head (72be432).
Report is 3 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #14884   +/-   ##
=======================================
  Coverage   95.85%   95.85%           
=======================================
  Files        1960     1960           
  Lines       25562    25565    +3     
  Branches     2889     2889           
=======================================
+ Hits        24502    24505    +3     
  Misses        799      799           
  Partials      261      261

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JamalAlabdullah JamalAlabdullah mentioned this pull request Mar 11, 2025
Open
3 tasks
@mlqn mlqn marked this pull request as ready for review March 11, 2025 17:24
@mlqn mlqn changed the title refactor: navigation contexts refactor: Wrap the new FormDesignerNavigation with AppContext Mar 11, 2025
@JamalAlabdullah JamalAlabdullah self-assigned this Mar 11, 2025
JamalAlabdullah
JamalAlabdullah approved these changes Mar 12, 2025
View reviewed changes
Copy link
Contributor

@JamalAlabdullah JamalAlabdullah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 👍

@JamalAlabdullah JamalAlabdullah removed their assignment Mar 12, 2025
@mlqn mlqn added the skip-manual-testing PRs that do not need to be tested manually label Mar 12, 2025
@mlqn mlqn merged commit b486c5f into main Mar 12, 2025
12 checks passed
@mlqn mlqn deleted the chore/navigation-refactoring branch March 12, 2025 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] approved these changes

@JamalAlabdullah JamalAlabdullah JamalAlabdullah approved these changes

Assignees
No one assigned
Labels
area/ui-editor Area: Related to the designer tool for assembling app UI in Altinn Studio. frontend skip-manual-testing PRs that do not need to be tested manually solution/studio/designer Issues related to the Altinn Studio Designer solution. team/studio-domain2
Projects
Team Studio
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mlqn @JamalAlabdullah